-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CJS-5: Remove immutable #602
Conversation
…into remove-immutable
…into remove-immutable
…s of self documenting code styles
…into remove-immutable
…into remove-immutable
…into remove-immutable
…into remove-immutable
Size Change: -3.07 kB (1%) Total Size: 214 kB
|
…into remove-immutable
adding types for translations
src/types.ts
Outdated
@@ -32,6 +31,14 @@ export type RequireOnlyOne<T, Keys extends keyof T = keyof T> = Pick< | |||
[K in Keys]-?: Required<Pick<T, K>> & Partial<Record<Exclude<Keys, K>, undefined>>; | |||
}[Keys]; | |||
|
|||
export type RequireOnlyTwo<T, Keys extends keyof T = keyof T> = Pick< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it used somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, we thought originally there'd be only 2 translation texts { en_text: 'Something', es_text: 'Translated' }
but there can be more than that. but after this type rule was figured out so we left it just in case it's needed in the future for anything as it was a bit of a thought process to get to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's not in master, might be good to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ferhatelmas @vishalnarkhede removed
…into remove-immutable
if (oldReactionCounts) { | ||
const oldReactionType = oldReactionCounts[reaction.type]; | ||
oldReactionCounts[reaction.type] = oldReactionType ? oldReactionType - 1 : 0; | ||
messageWithReaction.reaction_counts = oldReactionCounts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is a responsibility of _removeReactionFromMessage
which would also prevent duplication below
Pointing this but it might actually be out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah, I think maybe it's for another PR so we can keep the scope of this to immutables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Submit a pull request
CLA
Description of the pull request